Skip to content

fix: clean up failed Skill Card attachments - #3614

Merged
steipete merged 1 commit into
openclaw:mainfrom
SebTardif:fix/skill-card-store-cleanup
Sep 16, 2026
Merged

fix: clean up failed Skill Card attachments#3614
steipete merged 1 commit into
openclaw:mainfrom
SebTardif:fix/skill-card-store-cleanup

Conversation

@SebTardif

@SebTardif SebTardif commented Sep 5, 2026 •
edited by steipete
Loading

Copy link
Copy Markdown
Contributor

Ref #3674.

What Problem This Solves

Fixes generated Skill Card blobs being left in storage when attachment rejects a stale worker lease or an unavailable skill version.

User Impact

Failed attachments clean up their newly generated blob. Successfully attached cards, earlier card blobs, and historical bundle fingerprints remain available.

Why This Change Was Made

The completion action owns the newly stored blob until the attachment mutation succeeds. A rejected mutation now triggers cleanup of that blob only, while retaining the original attachment error if cleanup also fails.

Evidence

  • Registered Convex function/storage fixtures cover stale leases, soft-deleted and missing versions, successful replacement, job state, and historical fingerprint retention. All 17 focused tests pass. Removing cleanup reproduces three orphan-blob failures.
  • Real isolated local Convex and Chromium publish/card lifecycle: a stale lease leaves the storage ID set unchanged; successful completion adds one blob and the generated card is readable through the HTTP endpoint and page. The test passed in 34.3 seconds.
  • Full bun run ci:pr passed before the main refresh, including 6,894 coverage tests, package verification, types/build, and HTTP integration. After rebasing, the 17 focused tests, full static gate, and full types/build gate passed again.
  • Independent review through P2 is scoped-clean. Maintainer follow-up and additional validation were AI-assisted.

@clawsweeper

clawsweeper Bot commented Sep 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@vercel

vercel Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

@SebTardif is attempting to deploy a commit to the OpenClaw Foundation Team on Vercel.

A member of the Team first needs to authorize it.

@clawsweeper clawsweeper Bot added P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 5, 2026
@clawsweeper

clawsweeper Bot commented Sep 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 15, 2026, 11:49 PM ET / September 16, 2026, 03:49 UTC (Revision 3).

ClawSweeper review

What this changes

Delete newly generated Skill Card files when attachment fails, with regression coverage and documentation preserving successful cards and historical fingerprints.

Merge readiness

✅ Ready for maintainer review

This PR remains necessary: current main still leaks newly stored cards when attachment rejects. No blocking findings remain, and the updated local Convex evidence addresses the previous proof request.

Priority: P2
Reviewed head: e8795ffbd68d86a969ced09ac9188f4f970da0cc

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused repair with relevant runtime observations, regression coverage, and no blocking correctness or security findings.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (live_output): The updated body reports real isolated Convex/Chromium execution of completion: stale-lease rejection leaves storage unchanged, success retains one new blob, and HTTP/page reads succeed. These observations match the inspected lifecycle assertions and resolve the prior request; the matching head’s runtime job passed, although raw job logs were inaccessible to this reviewer.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The updated body reports real isolated Convex/Chromium execution of completion: stale-lease rejection leaves storage unchanged, success retains one new blob, and HTTP/page reads succeed. These observations match the inspected lifecycle assertions and resolve the prior request; the matching head’s runtime job passed, although raw job logs were inaccessible to this reviewer.
Evidence reviewed 10 items Current main still needs the repair: The completion action stores Markdown and awaits attachment without cleanup. The attachment mutation rejects mismatched leases and missing or soft-deleted versions.
Release comparison: The v0.23.3 source also stores the card without failure cleanup; no already-shipped implementation was established.
Introduced cleanup and retained invariants: The catch deletes only the storage ID allocated by this invocation and rethrows the original error. Authentication, attachment authorization, successful replacement, and historical fingerprint retention remain unchanged. The registered-function tests cover three rejection cases and successive successful replacements.
Findings None None.
Security None None.

How this fits together

ClawHub’s Skill Card worker submits generated Markdown to a Convex action, which stores it and attaches it to a skill version. Attached cards become available through the catalog’s HTTP endpoint and Skill Card tab.

flowchart TD
  A[Worker submits Markdown] --> B[Validate worker and content]
  B --> C[Store new card]
  C --> D{Attachment succeeds?}
  D -->|Yes| E[Retain card and fingerprints]
  E --> F[HTTP endpoint and catalog page]
  D -->|No| G[Delete new blob and preserve error]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Diff size +175/-12 across 6 files The scope includes one production function, regression coverage, and supporting documentation.
Production versus tests Production +5 net; tests +154 The small production increase closes the allocation leak, while most growth validates cleanup and retention.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #3674
Summary: This PR implements the repair described by the open Skill Card leak tracker.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Technical review

Best possible solution:

Keep cleanup with the action that allocates each card, preserving attachment errors and existing card and fingerprint retention.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: main stores a blob before a mutation that rejects stale leases or unavailable versions, with no cleanup. This review did not execute a current-main reproduction.

Is this the best way to solve the issue?

Yes—the allocating action is the narrowest repair location and matches the existing upload cleanup pattern. Moving cleanup into the external worker would lose access to the newly allocated storage ID.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 7406f65e792f.

Labels

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The updated body reports real isolated Convex/Chromium execution of completion: stale-lease rejection leaves storage unchanged, success retains one new blob, and HTTP/page reads succeed. These observations match the inspected lifecycle assertions and resolve the prior request; the matching head’s runtime job passed, although raw job logs were inaccessible to this reviewer.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The updated body reports real isolated Convex/Chromium execution of completion: stale-lease rejection leaves storage unchanged, success retains one new blob, and HTTP/page reads succeed. These observations match the inspected lifecycle assertions and resolve the prior request; the matching head’s runtime job passed, although raw job logs were inaccessible to this reviewer.
  • remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.

Label justifications:

  • P2: Failed background card attachments leak storage, with limited blast radius and no demonstrated user data loss.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The updated body reports real isolated Convex/Chromium execution of completion: stale-lease rejection leaves storage unchanged, success retains one new blob, and HTTP/page reads succeed. These observations match the inspected lifecycle assertions and resolve the prior request; the matching head’s runtime job passed, although raw job logs were inaccessible to this reviewer.
  • proof: sufficient: Contributor real behavior proof is sufficient. The updated body reports real isolated Convex/Chromium execution of completion: stale-lease rejection leaves storage unchanged, success retains one new blob, and HTTP/page reads succeed. These observations match the inspected lifecycle assertions and resolve the prior request; the matching head’s runtime job passed, although raw job logs were inaccessible to this reviewer.

Evidence

What I checked:

  • Current main still needs the repair: The completion action stores Markdown and awaits attachment without cleanup. The attachment mutation rejects mismatched leases and missing or soft-deleted versions. (convex/skillCards.ts:529, 7406f65e792f)
  • Release comparison: The v0.23.3 source also stores the card without failure cleanup; no already-shipped implementation was established. (convex/skillCards.ts, 87ca030c30f3)
  • Introduced cleanup and retained invariants: The catch deletes only the storage ID allocated by this invocation and rethrows the original error. Authentication, attachment authorization, successful replacement, and historical fingerprint retention remain unchanged. The registered-function tests cover three rejection cases and successive successful replacements. (convex/skillCards.cleanup.runtime.test.ts:55, e8795ffbd68d)
  • Updated real runtime evidence: The captured PR body reports an isolated local Convex/Chromium lifecycle completing in 34.3 seconds: stale-lease rejection leaves storage IDs unchanged, success adds one blob, and HTTP/page reads return the card. The live REST body matches the supplied snapshot. The added lifecycle assertions exercise precisely those outcomes through ConvexHttpClient and actual storage queries. (e2e/local-auth/publish-skill-lifecycle.pw.test.ts:390, e8795ffbd68d)
  • Matching runtime job corroboration: The publish-generated-card job and its Local-auth browser e2e step succeeded on the pinned head: https://github.com/openclaw/clawhub/actions/runs/35052936380/job/104657023664. The workflow selects this lifecycle test, and its runner starts isolated Convex and pushes the functions. Raw log retrieval was blocked by the environment’s artifact-host allowlist; no raw trace was inspected. (.github/workflows/ci.yml:202, e8795ffbd68d)
  • Existing cleanup pattern and alternative: The skill upload HTTP path already deletes a newly stored blob when attachment fails. Keeping cleanup in the allocating action follows that pattern; worker-side cleanup would lack the storage ID when completion throws. (convex/httpApiV1/skillsV1.ts:3068, e8795ffbd68d)

Likely related people:

  • Patrick-Erichsen: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • jesse-merhi: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)
  • reviewed 2026-09-05T16:41:04.470Z sha 2d28ce6 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-11T14:08:28.254Z sha 2d28ce6 :: needs real behavior proof before merge. :: none

This was referenced Sep 7, 2026
Open
Closed
@steipete @SebTardif
fix: clean up failed Skill Card attachments
e8795ff 
Delete only the new card blob when attachment rejects, preserve the original failure, and retain successful cards and historical fingerprints. Prove storage contents through registered Convex fixtures and the real local-auth publish lifecycle.

Co-authored-by: Sebastien Tardif <SebTardif@ncf.ca>
@steipete
steipete force-pushed the fix/skill-card-store-cleanup branch from 2d28ce6 to e8795ff Compare September 16, 2026 03:44
@steipete
steipete requested review from a team and Patrick-Erichsen as code owners September 16, 2026 03:44
@steipete steipete changed the title fix: delete skill-card blob when attach fails fix: clean up failed Skill Card attachments Sep 16, 2026
@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Sep 16, 2026
@steipete
steipete merged commit 3ab0037 into openclaw:main Sep 16, 2026
55 of 57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Patrick-Erichsen Patrick-Erichsen Awaiting requested review from Patrick-Erichsen Patrick-Erichsen is a code owner

Assignees

No one assigned

Labels

P2 Normal backlog priority with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@SebTardif @steipete